Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(spanner): unflake unit tests #3584

Merged
merged 1 commit into from
Jan 8, 2025
Merged

chore(spanner): unflake unit tests #3584

merged 1 commit into from
Jan 8, 2025

Conversation

sakthivelmanii
Copy link
Contributor

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> ☕️

If you write sample code, please follow the samples format.

@product-auto-label product-auto-label bot added size: xs Pull request size is extra small. api: spanner Issues related to the googleapis/java-spanner API. labels Jan 6, 2025
@sakthivelmanii sakthivelmanii force-pushed the unflake_unit_tests branch 2 times, most recently from 7c8c91a to e552106 Compare January 6, 2025 07:40
@product-auto-label product-auto-label bot added size: u Pull request is empty. and removed size: xs Pull request size is extra small. labels Jan 6, 2025
@product-auto-label product-auto-label bot added size: s Pull request size is small. and removed size: u Pull request is empty. labels Jan 6, 2025
@sakthivelmanii sakthivelmanii changed the title unflake unit tests fix(spanner): unflake unit tests Jan 6, 2025
@sakthivelmanii sakthivelmanii force-pushed the unflake_unit_tests branch 2 times, most recently from 27f72a3 to 62cdd43 Compare January 6, 2025 10:21
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Jan 6, 2025
@sakthivelmanii sakthivelmanii force-pushed the unflake_unit_tests branch 5 times, most recently from 7c869a5 to 46d6e7d Compare January 6, 2025 13:47
@product-auto-label product-auto-label bot added size: xs Pull request size is extra small. size: s Pull request size is small. and removed size: m Pull request size is medium. size: xs Pull request size is extra small. labels Jan 6, 2025
@sakthivelmanii sakthivelmanii force-pushed the unflake_unit_tests branch 5 times, most recently from 05b9d1b to 22149da Compare January 6, 2025 14:52
@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Jan 6, 2025
@product-auto-label product-auto-label bot added size: s Pull request size is small. and removed size: xs Pull request size is extra small. labels Jan 6, 2025
@product-auto-label product-auto-label bot added size: xs Pull request size is extra small. and removed size: s Pull request size is small. labels Jan 6, 2025
@product-auto-label product-auto-label bot added size: s Pull request size is small. and removed size: xs Pull request size is extra small. labels Jan 6, 2025
@sakthivelmanii sakthivelmanii marked this pull request as ready for review January 6, 2025 18:12
@sakthivelmanii sakthivelmanii requested a review from a team as a code owner January 6, 2025 18:12
Copy link
Collaborator

@olavloite olavloite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks promising! I have one question regarding whether this is actually a synchronization problem (so needing a lock), or just a caching problem (meaning; making it volatile should be enough).

// When start a new stream set the Span as current to make the gRPC Span a child of
// this Span.
stream = checkNotNull(startStream(resumeToken, streamMessageListener));
synchronized (monitor) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need the lock in this method? As in: Is it really possible that two threads try to call this method simultaneously? Or is the problem just that there are two threads calling this method quickly after each other, and one of them does not (always) see the result of the previous call by another thread? And would in the latter case just making stream volatile be enough?

The reason for asking is mainly because:

  1. If it is so that multiple threads can call this method simultaneously, from where does that happen? (If it is guaranteed that it does not happen in parallel, then the rest of this can be ignored)
  2. If that happens from both computeNext() and initiateStreaming(), then a potential follow-on problem could be that the call from computeNext() is executed first, and that will then not set a value for streamMessageListener.
  3. And otherwise; If both parallel calls come through initiateStreaming(); Could we instead synchronize those calls? Or somehow prevent that from happening in parallel?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

volatile didn't actually fix the issue. Let me try to figure out the issue at the root.

Copy link
Contributor Author

@sakthivelmanii sakthivelmanii Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue:

  1. We are trying to request for initial chunks in startStream which returns the stream.
  2. We are trying to request chunks before stream object returned to startGrpcStreaming in ResumableStreamIterator
  3. Because of this, chunk is received first and it created ProduceRowsRunnable thread and internally it called GrpcResultSet#next() and that internally ResumableStreamIterator#computeNext() which creates the stream if it's null
  4. Due to this reason, 2 begin transaction were started which is causing the the test failure.

@olavloite I have shared the events with you in chat which confirms this behaviour.

@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Jan 6, 2025
@sakthivelmanii sakthivelmanii force-pushed the unflake_unit_tests branch 2 times, most recently from 0b0c3a2 to baf2b5a Compare January 6, 2025 20:40
@product-auto-label product-auto-label bot added size: s Pull request size is small. and removed size: m Pull request size is medium. labels Jan 6, 2025
@sakthivelmanii sakthivelmanii force-pushed the unflake_unit_tests branch 3 times, most recently from 6c54b63 to aedcfd1 Compare January 7, 2025 03:49
@sakthivelmanii sakthivelmanii changed the title fix(spanner): unflake unit tests chore(spanner): unflake unit tests Jan 8, 2025
@rahul2393 rahul2393 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 8, 2025
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 8, 2025
@rahul2393 rahul2393 merged commit 7e27aca into main Jan 8, 2025
34 checks passed
@rahul2393 rahul2393 deleted the unflake_unit_tests branch January 8, 2025 06:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/java-spanner API. size: s Pull request size is small.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants